🐛 report Unknown for deprecation status when catalog unavailable and prevent error leak #2296
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors deprecation status handling in ClusterExtension reconciliation to ensure deprecation conditions accurately reflect catalog data and installed bundle state, preventing install/validation errors from leaking into deprecation conditions.
- Moved deprecation status updates to a deferred function that runs at the end of reconciliation
- Changed BundleDeprecated condition to use
Unknownstatus withReasonAbsentwhen no bundle is installed - Updated test expectations to handle multiple possible error messages for sourceType validation
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/operator-controller/controllers/clusterextension_controller.go | Refactored deprecation status logic with deferred updates and new condition semantics for uninstalled bundles |
| internal/operator-controller/controllers/clusterextension_controller_test.go | Added tests for deprecation handling with resolution failures and applier failures; updated test expectations for BundleDeprecated conditions |
| test/e2e/cluster_extension_install_test.go | Updated e2e tests to verify deprecation conditions in success and failure scenarios |
| internal/operator-controller/controllers/clusterextension_admission_test.go | Enhanced sourceType validation test to handle multiple possible error message formats |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/controllers/clusterextension_controller.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/controllers/clusterextension_admission_test.go
Outdated
Show resolved
Hide resolved
camilamacedo86
force-pushed
the
fix-issue-conditions
branch
from
0891f37 to
4f61e6a
Compare
camilamacedo86
force-pushed
the
fix-issue-conditions
branch
from
4f61e6a to
2b26273
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/controllers/clusterextension_controller.go
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2296 +/- ##
==========================================
+ Coverage 74.30% 74.41% +0.11%
==========================================
Files 91 91
Lines 7083 7118 +35
==========================================
+ Hits 5263 5297 +34
- Misses 1405 1407 +2
+ Partials 415 414 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
camilamacedo86
force-pushed
the
fix-issue-conditions
branch
from
2b26273 to
ad9ccfd
Compare
camilamacedo86
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
internal/operator-controller/controllers/clusterextension_controller_test.go:1
- The assertion
require.Contains(ct, []string{...}, cond.Reason)is semantically backwards. TheContainsfunction expects a slice as the second argument and the search item as the first. This should berequire.Contains(ct, cond.Reason, []string{ocv1.ReasonFailed, ocv1.ReasonAbsent})or userequire.True(ct, slices.Contains([]string{ocv1.ReasonFailed, ocv1.ReasonAbsent}, cond.Reason)).
package controllers_test
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/controllers/clusterextension_controller.go
Outdated
Show resolved
Hide resolved
camilamacedo86
force-pushed
the
fix-issue-conditions
branch
from
ad9ccfd to
a6d0678
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/controllers/clusterextension_controller.go
Outdated
Show resolved
Hide resolved
camilamacedo86
force-pushed
the
fix-issue-conditions
branch
from
a6d0678 to
287bede
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/controllers/clusterextension_controller.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/controllers/clusterextension_controller.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/controllers/clusterextension_controller.go
Outdated
Show resolved
Hide resolved
camilamacedo86
force-pushed
the
fix-issue-conditions
branch
from
287bede to
b2da76f
Compare
internal/operator-controller/controllers/clusterextension_controller.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/controllers/clusterextension_controller.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/controllers/clusterextension_controller.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
camilamacedo86
force-pushed
the
fix-issue-conditions
branch
from
0f8d723 to
351b6cc
Compare
camilamacedo86
force-pushed
the
fix-issue-conditions
branch
from
351b6cc to
be235b3
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
camilamacedo86
force-pushed
the
fix-issue-conditions
branch
from
be235b3 to
1707572
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Not sure if this has been addressed in other comments (if so, please update the PR description!). I noticed that we have
|
|
One thing I didn't see answered in the description is the open question in the original TODO comment about what happens when the package and/or channel is present in multiple catalogs, but we didn't resolve a bundle:
|
|
Is there an RFC for this? It kinda feels like we should have something written up in that form? I also wonder if we should tackle the question of whether to explicitly include the Deprecation conditions when we would set them as False. They add a lot of noise to the YAML/JSON and |
camilamacedo86
force-pushed
the
fix-issue-conditions
branch
from
1707572 to
22c0e0e
Compare
camilamacedo86
force-pushed
the
fix-issue-conditions
branch
from
22c0e0e to
61faa25
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @joelanford Thanks for the feedback! You're absolutely right - we should keep the TODO and open question for item 2. I've done that. See the updated TODO with scenarios here: internal/operator-controller/controllers/clusterextension_controller.go The core logic for handling multiple catalogs hasn't changed - the resolver still picks one. What changed is we now correctly handle the failure cases and don't lose deprecation information when things go wrong. For that yes, I think we need a follow up PR/discussion. ( out of scope ) Just to clarify the scope: this PR solves item 1 (use installed bundle instead of resolved) and #2008 (install errors leaking into deprecation conditions). The open question about conflicting deprecations from multiple catalogs is a separate edge case that deserves its own focused discussion and PR (IHMO, lets go step by step). Key Differences (What This PR Fixes)
Hope this addresses your concern! Let me know if you'd like me to clarify anything else. |
camilamacedo86
force-pushed
the
fix-issue-conditions
branch
from
61faa25 to
2d78eed
Compare
… and prevent error leak When a catalog is removed or unreachable, deprecation conditions now correctly show Unknown instead of False. BundleDeprecated now reflects the installed bundle and uses the correct reason. Assisted-by: Cursor
camilamacedo86
force-pushed
the
fix-issue-conditions
branch
from
2d78eed to
d84278c
Compare
5 participants
Fix install errors leaking into deprecation conditions
Fixes deprecation conditions incorrectly showing install/validation errors instead of actual deprecation status.
Closes #2008
What was broken
When bundle installation or validation failed, the error message was getting copied to all conditions including the deprecation ones. This made deprecation status useless and confusing.
Example from the bug report - when a bundle failed validation:
The deprecation conditions should show deprecation information from the catalog, not installation errors.
While fixing this, we also discovered and fixed several other related issues:
Falsewhen catalog was removed (should beUnknown)Falsefor deprecation even though no catalog was checkedKey Differences (What This PR Fixes)
False❌ (wrong!)Unknown✅What you'll see now
Example 1: The original bug - validation failure
Before this fix:
After this fix:
Install errors only appear in
ProgressingandInstalledconditions.Example 2: Catalog gets removed
Before this fix:
After this fix:
Example 3: Upgrading v1→v2
Before this fix:
After this fix:
Example 4: Resolution fails but package is deprecated
Before this fix:
After this fix:
Behavior summary
Closes #2008